Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: refactor AuroraTestUtility #1252

Merged
merged 23 commits into from
Jan 23, 2025
Merged

chore: refactor AuroraTestUtility #1252

merged 23 commits into from
Jan 23, 2025

Conversation

aaron-congo
Copy link
Contributor

Description

  • removes most instance fields in AuroraTestUtility, in particular the cluster details. Some functions were relying on these details being passed in and others were relying on internal state set by previous functions, making it difficult to understand which details were in use
  • addresses some small IDE warnings/suggestions in the code

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

github-actions bot commented Jan 22, 2025

Qodana Community for JVM

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/[email protected]
        with:
          upload-result: true
Contact Qodana team

Contact us at [email protected]

final TestInstanceInfo newInstance =
auroraUtil.createInstance("auto-scaling-instance");
auroraUtil.createInstance(instanceClass, "auto-scaling-instance");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before these changes, the new instance was added to the TestEnvironment in a convoluted way: an earlier call to AuroraTestUtility would pass in the TestEnvironment instance list, which the class would then assign to an internal instance variable. Then, in this call, the instance list would be updated. In this PR we removed the internal instance variable. Should I change the method signature to require the TestEnvironment instance list to be passed in so that we can update it inside the function instead of relying on callers to update it themselves? It would be convenient but it also would be a side effect of calling the function

@@ -857,7 +857,7 @@ public void test_pooledConnection_differentUsers() throws SQLException {

assertThrows(
HikariPool.PoolInitializationException.class, () -> {
try (final Connection conn = DriverManager.getConnection(
try (final Connection ignored = DriverManager.getConnection(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just fixing an intellij warning here

@@ -935,7 +953,7 @@ public void makeSureInstancesUp(List<TestInstanceInfo> instances, long timeoutSe
host,
instanceInfo.getPort(),
dbName);
try (final Connection conn = DriverManager.getConnection(url, props)) {
try (final Connection ignored = DriverManager.getConnection(url, props)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just getting rid of an intellij warning here

@aaron-congo aaron-congo merged commit 8bec698 into main Jan 23, 2025
10 checks passed
@aaron-congo aaron-congo deleted the test-utility branch January 23, 2025 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants